enhance: refactor delete mvcc function (#38066) #39258
Conversation
|
@zhagnlu Thanks for your contribution. Please submit with DCO, see the contributing guide https://github.com/milvus-io/milvus/blob/master/CONTRIBUTING.md#developer-certificate-of-origin-dco. |
|
@zhagnlu Please associate the related pr of master to the body of your Pull Request. (eg. “pr: #”) |
mergify
bot
added
do-not-merge/missing-related-pr
kind/enhancement
zhagnlu
force-pushed
the
refactor_delete_2.4
branch
from
62492c9 to
971b7d9
Compare
mergify
bot
added
dco-passed
zhagnlu
force-pushed
the
refactor_delete_2.4
branch
from
971b7d9 to
5f134d8
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## 2.4 #39258 +/- ##
==========================================
+ Coverage 79.50% 83.44% +3.93%
==========================================
Files 1077 811 -266
Lines 169767 143417 -26350
==========================================
- Hits 134974 119674 -15300
+ Misses 30297 19244 -11053
- Partials 4496 4499 +3 |
| } | ||
| for (auto& offset : offsets) { | ||
| auto row_id = offset.get(); | ||
| // if alreay deleted, no need to add new record |
There was a problem hiding this comment.
I'm not sure if ignoring deleted entries are safe or not. Say we have the following sequence of operations:
DELETE (K1, T0)
INSERT (K1, T1)
DELETE (K1, T2)
do you mean the 3rd operation is ignored here?
There was a problem hiding this comment.
when delete, all k1 will convert to [offset, ts], not just [k1, ts], so the 3rd operation will do:
- find all exist offset that meet pk is k1, include 2rd insert, and 2rd insert is a new offset
- judge whether offsets from 1 is deleted or not. if offset is deleted, not add new record, if not, add new record.
so the 3rd not fully ignored
| if (!snapshots_.empty()) { | ||
| int loc = snapshots_.size() - 1; |
There was a problem hiding this comment.
The access of snapshots_ here is not protected by lock snap_lock_, is it OK?
There was a problem hiding this comment.
good catch, need to lock it actully, I will move the lock below to the top
Signed-off-by: luzhang <[email protected]>
zhagnlu
force-pushed
the
refactor_delete_2.4
branch
from
5f134d8 to
376724a
Compare
|
/lgtm |
| std::move(segment_->search_pk(deleted_pk, deleted_ts)); | ||
| } else { | ||
| // only for testing | ||
| offsets = std::move( |
There was a problem hiding this comment.
Do not add test code here in the future.
You should encapsulate the search_pk as a function pointer within DeleteRecord. Pass the function object from outside.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: czs007, zhagnlu The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
pr: #38066, #38509, #38549, #38580, #38991 cherry-pick from master